-
Notifications
You must be signed in to change notification settings - Fork 1.4k
RSP-1263 Documentation for StatusLight #194
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| prompting a response, or distinguishing a visual element." | ||
| If StatusLight communicates a state, like "Approved" or "Failed", that state must be included as part of the label. | ||
|
|
||
| #### Using role="status" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a follow up, we'll probably have to support h4's so they get properly styled in the Layout. No need to change here, just wanted to note
|
@pst67662 Ah forgot to mention, but you'll probably want to change this pull to point to |
|
Build successful! 🎉 |
|
@LFDanLu I tried to rebase and ended up with commits from docs branch :/ sooo I'll reopen this PR |
|
@pst67662 Ah, you can always try |
|
|
df71ba6 to
c2d99be
Compare
|
Build successful! 🎉 |
|
|
||
| ## Content | ||
|
|
||
| A visible label can be provided by passing children. Spectrum dictates that it should always be used with a label, never alone. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should say 'Spectrum dictates'.
Maybe:
A visible label is always required and can be provided by passing children.
|
|
||
| ### Accessibility | ||
|
|
||
| If no visible label is provided (e.g. an icon only button), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In content you said a visible label is always required?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm I need a little help with this one... we define children as required in react-types, but then we also check for aria-label if there are no children in StatusLight.tsx: https://github.com/adobe-private/react-spectrum-v3/blob/master/packages/%40react-spectrum/statuslight/src/StatusLight.tsx#L18
should label/children be optional then? @majornista
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Label/children should not be optional, but the warning is helpful.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is something like "If textual child is not provided (e.g. an icon only)" OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example for "aria-label" added
|
Build successful! 🎉 |
LFDanLu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 small suggestion and a question but otherwise looks good to me.
|
Build successful! 🎉 |
|
Build successful! 🎉 |
LFDanLu
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Build successful! 🎉 |
|
Build successful! 🎉 |
Closes
✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Team: